-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: upgrade Clang requirement to 3.4.2 #12388
Conversation
Clang 3.4.1 has a bug that prevents compilation of V8. As of Node.js 8.0.0 we stopped floating a workaround for this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and good catch on configure.
I'd be okay with raising the baseline to 3.5.2 or 3.6.2 for node 8. We'll be supporting it until 2021, it would make sense to start with a compiler that's not already long in the tooth.
In terms 3.5.2 and 3.6.2 will those run on the existing OS levels were Clang is used in our regression runs ? |
I'm all for raising the bar in node 8 as well. ubuntu trusty seems to offer clang 3.4, 3.5, 3.6 and 3.8. Any arguments on 3.5.x vs 3.6.x? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'd say so. I don't think we have any systems in our matrix where you can install 3.4 but not 3.5 or 3.6.
PPC support got a big boost in 3.6. Might be worth it for that reason alone. |
Sounds good to me. The newer, the better. |
CI since there's a change in the configure script here: https://ci.nodejs.org/job/node-test-pull-request/7489/ |
CI failure is unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Clang 3.4.1 has a bug that prevents compilation of V8. As of Node.js 8.0.0 we stopped floating a workaround for this issue. PR-URL: #12388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Landed in b4f59a7 |
I'm assuming this doesn't apply to v7 and earlier, LMK if that's not correct. |
We carry a compatibility patch in older branches although it's no longer tested now that the clang 3.4.1 buildbot has been decommissioned. I'd back-port this PR, that way people know what the known-good version of clang is. |
Clang 3.4.1 has a bug that prevents compilation of V8. As of Node.js 8.0.0 we stopped floating a workaround for this issue. PR-URL: #12388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Backported in 11a7875 |
Clang 3.4.1 has a bug that prevents compilation of V8. As of Node.js 8.0.0 we stopped floating a workaround for this issue. PR-URL: #12388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.
I don't have a test system to check if 3.4.2 is enough.
/cc @nodejs/build
Checklist